-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
do not wipe out existing connection resource requirements on update #10291
do not wipe out existing connection resource requirements on update #10291
Conversation
@@ -74,7 +79,21 @@ public ConnectionRead updateConnection(final ConnectionUpdate connectionUpdate) | |||
.withMemoryRequest(connectionUpdate.getResourceRequirements().getMemoryRequest()) | |||
.withMemoryLimit(connectionUpdate.getResourceRequirements().getMemoryLimit())); | |||
} else { | |||
newConnection.withResourceRequirements(workerConfigs.getResourceRequirements()); | |||
final io.airbyte.config.ResourceRequirements resourceRequirements = Optional.ofNullable(persistedSync.getResourceRequirements()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we want to leave them blank so we use the workerConfig at job creation time, not in the connection update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah you're right, this class should not be pulling resource requirements from the worker configs at all. I will update
What
Today when a connection is updated, its currently-set
resource_requirements
are wiped out if the update input does not include new resource requirements.This is bad because we are currently using connection
resource_requirements
to bump the resource limits of a connection, and wiping these out can lead to more failures. This has already happened once: https://airbytehq-team.slack.com/archives/C02U46QK5C3/p1644618998737659?thread_ts=1644599694.890299&cid=C02U46QK5C3How
This PR solves the issue by adding logic to use already-existing resource_requirement values on connection update if the update input does not include new values.